wip(agents-api): Add Doc sql queries #979
Draft
+1,740
−449
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
CI Failure Feedback 🧐(Checks updated until commit 3600a92)
✨ CI feedback usage guide:The CI feedback tool (
In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR: where Configuration options
See more information about the |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 6c77490 in 1 minute and 30 seconds
More details
- Looked at
848lines of code in10files - Skipped
0files when reviewing. - Skipped posting
8drafted comments based on config settings.
1. agents-api/agents_api/queries/docs/search_docs_hybrid.py:13
- Draft comment:
The import statement forrun_concurrentlyis unnecessary since it's not used in the code. Consider removing it to clean up the imports. - Reason this comment was not posted:
Confidence changes required:10%
The import statement forrun_concurrentlyis unnecessary since it's not used in the code.
2. agents-api/agents_api/queries/docs/search_docs_hybrid.py:134
- Draft comment:
Appending an empty list totasksis incorrect. You should append a coroutine or task. Consider usingNoneor a similar placeholder if you intend to skip adding a task. - Reason this comment was not posted:
Confidence changes required:50%
The code usestasks.append([])which is incorrect for appending tasks. It should append a coroutine instead.
3. agents-api/agents_api/queries/docs/search_docs_hybrid.py:147
- Draft comment:
Appending an empty list totasksis incorrect. You should append a coroutine or task. Consider usingNoneor a similar placeholder if you intend to skip adding a task. - Reason this comment was not posted:
Confidence changes required:50%
The code usestasks.append([])which is incorrect for appending tasks. It should append a coroutine instead.
4. agents-api/agents_api/queries/docs/search_docs_hybrid.py:154
- Draft comment:
Ensure that at least one valid task is added totasksbefore callinggather. If bothtext_queryandembeddingare empty,taskswill contain only empty lists, leading to an error. - Reason this comment was not posted:
Confidence changes required:50%
The code does not handle the case where bothtext_queryandembeddingare empty, which could lead to an error when callinggather.
5. agents-api/agents_api/queries/docs/search_docs_hybrid.py:110
- Draft comment:
Theembeddingparameter should have a default value of an empty list[]instead ofNoneto avoid type issues and simplify checks. - Reason this comment was not posted:
Confidence changes required:50%
Theembeddingparameter should have a default value of an empty list instead ofNoneto avoid type issues.
6. agents-api/agents_api/queries/docs/create_doc.py:69
- Draft comment:
Consider adding more exception handling for other potential database errors, such asasyncpg.DataErrororasyncpg.SyntaxOrAccessError, to make the function more robust. - Reason this comment was not posted:
Confidence changes required:50%
Therewrap_exceptionsdecorator is used to handle exceptions, but it doesn't cover all possible exceptions that might occur during database operations.
7. agents-api/agents_api/queries/docs/delete_doc.py:43
- Draft comment:
Consider adding more exception handling for other potential database errors, such asasyncpg.DataErrororasyncpg.SyntaxOrAccessError, to make the function more robust. - Reason this comment was not posted:
Confidence changes required:50%
Therewrap_exceptionsdecorator is used to handle exceptions, but it doesn't cover all possible exceptions that might occur during database operations.
8. agents-api/agents_api/queries/docs/get_doc.py:29
- Draft comment:
Consider adding exception handling for potential database errors, such asasyncpg.DataErrororasyncpg.SyntaxOrAccessError, to make the function more robust. - Reason this comment was not posted:
Confidence changes required:50%
Therewrap_exceptionsdecorator is used to handle exceptions, but it doesn't cover all possible exceptions that might occur during database operations.
Workflow ID: wflow_0SgPnESfFL0Scwfr
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| return similarity | ||
|
|
||
|
|
||
| def maximal_marginal_relevance( |
There was a problem hiding this comment.
This is an exact duplicate of the existing implementation.
- function
maximal_marginal_relevance(mmr.py)
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 93673b7 in 1 minute and 3 seconds
More details
- Looked at
1195lines of code in21files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. agents-api/agents_api/queries/docs/get_doc.py:5
- Draft comment:
The import statement for 'asyncpg' is unnecessary and can be removed to clean up the code. This is also applicable in other files like 'list_docs.py', 'search_docs_by_embedding.py', and 'search_docs_by_text.py'. - Reason this comment was not posted:
Confidence changes required:50%
The import statement for 'asyncpg' is unnecessary in multiple files as it is not used anywhere in the code. Removing it will clean up the code.
2. agents-api/agents_api/queries/docs/list_docs.py:5
- Draft comment:
The import statement for 'asyncpg' is unnecessary and can be removed to clean up the code. This is also applicable in other files like 'search_docs_by_embedding.py', and 'search_docs_by_text.py'. - Reason this comment was not posted:
Confidence changes required:50%
The import statement for 'asyncpg' is unnecessary in multiple files as it is not used anywhere in the code. Removing it will clean up the code.
3. agents-api/agents_api/queries/docs/search_docs_by_embedding.py:6
- Draft comment:
The import statement for 'asyncpg' is unnecessary and can be removed to clean up the code. This is also applicable in 'search_docs_by_text.py'. - Reason this comment was not posted:
Confidence changes required:50%
The import statement for 'asyncpg' is unnecessary in multiple files as it is not used anywhere in the code. Removing it will clean up the code.
4. agents-api/agents_api/queries/docs/search_docs_by_text.py:6
- Draft comment:
The import statement for 'asyncpg' is unnecessary and can be removed to clean up the code. - Reason this comment was not posted:
Confidence changes required:50%
The import statement for 'asyncpg' is unnecessary in multiple files as it is not used anywhere in the code. Removing it will clean up the code.
5. agents-api/agents_api/queries/docs/create_doc.py:98
- Draft comment:
The 'org' option is removed from the owner_type Literal, but this change is not reflected in the PR description. This should be documented for clarity. This change is also applicable in 'delete_doc.py', 'get_doc.py', and 'list_docs.py'. - Reason this comment was not posted:
Confidence changes required:50%
The 'org' option is removed from the owner_type Literal in multiple files, but this change is not reflected in the PR description. This should be documented for clarity.
Workflow ID: wflow_pnbwiXEmTCJVnbrw
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on dc0ec36 in 48 seconds
More details
- Looked at
939lines of code in28files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. agents-api/agents_api/queries/users/delete_user.py:59
- Draft comment:
Theasyncpg.exceptions.UniqueViolationErrorshould not be handled here as it is not relevant to delete operations. Consider removing this exception handling. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_WUrKdAU41fxw9Cd0
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| status_code=404, | ||
| detail="The specified developer does not exist.", | ||
| ), | ||
| asyncpg.exceptions.UniqueViolationError: partialclass( |
There was a problem hiding this comment.
The asyncpg.exceptions.UniqueViolationError should not be handled here as it is not relevant to delete operations. Consider removing this exception handling.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 831e950 in 51 seconds
More details
- Looked at
440lines of code in11files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. agents-api/agents_api/queries/docs/get_doc.py:36
- Draft comment:
Usingast.literal_evalond["content"]can be unsafe if the content is not guaranteed to be a valid Python literal. Consider using a safer method to parse or handle the content. - Reason this comment was not posted:
Comment was on unchanged code.
2. agents-api/agents_api/queries/docs/embed_snippets.py:10
- Draft comment:
vectorizer_queryis set toNone. This is a placeholder and should be replaced with an actual query before deployment. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is essentially repeating information that's already explicitly stated in a TODO comment one line above. The TODO comment is more visible and serves the same purpose. This makes the PR comment redundant and not adding any new information or value.
Perhaps the comment is trying to emphasize the importance of not deploying with a None query, which could be a critical issue.
While deployment concerns are valid, the existing TODO comment already makes it clear this needs to be replaced, and deployment issues would be caught by basic testing since the function would fail immediately.
Delete the comment as it's redundant with the existing TODO comment and doesn't provide additional actionable value.
3. agents-api/agents_api/queries/entries/list_entries.py:88
- Draft comment:
Ensuresort_byanddirectionare validated and sanitized to prevent SQL injection, as they are used in string interpolation for SQL queries. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_TPwdOI1YwvBHnQ9P
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 249513d in 1 minute and 4 seconds
More details
- Looked at
1148lines of code in15files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. agents-api/agents_api/queries/docs/create_doc.py:153
- Draft comment:
The function assumesdata.contentis always a list. Consider adding a check or conversion to handle cases wheredata.contentmight be a string or other type to prevent unexpected behavior. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment suggests adding a check that already exists in the code. The code already handles both list and non-list cases appropriately through the isinstance() check and separate logic paths. The comment appears to be incorrect in stating that the function "assumes data.content is always a list" when it clearly doesn't make that assumption.
Could there be other types besides list and non-list that need handling? Could the CreateDocRequest type definition enforce the content type making this check unnecessary?
The code's else branch handles any non-list type appropriately by treating it as a single content item. The type validation would be handled by FastAPI's request validation via CreateDocRequest if needed.
The comment should be deleted because it incorrectly suggests adding a check that already exists in the code. The code already properly handles both list and non-list content.
2. agents-api/agents_api/queries/docs/delete_doc.py:25
- Draft comment:
TheEXISTSclause in the SQL query might be redundant since thedoc_ownersentry is already deleted in thedeleted_ownersCTE. Consider revising the logic to ensure thedocsentry is only deleted if thedoc_ownersentry existed prior to deletion. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment raises an interesting point about the query logic - we delete from doc_owners first, then check if that same record existed in doc_owners before deleting from docs. However, this could actually be intentional behavior to ensure atomicity and proper ordering. Without deeper knowledge of the data model and requirements, we can't be certain this is actually a problem vs a deliberate safeguard.
I may be missing important context about transaction isolation levels and race conditions that could make this pattern necessary. The EXISTS check might serve as an important guard rail.
While the comment raises an interesting point about query structure, we don't have enough context to confidently say this is incorrect or needs to be changed.
This comment is speculative and requires more context about the data model and requirements to validate. Following our rules, we should err on the side of removing speculative comments.
3. agents-api/agents_api/queries/docs/list_docs.py:135
- Draft comment:
Themetadata_filteris directly appended to the query string, which could lead to SQL injection if not properly handled. Ensure that metadata keys and values are safely included in the query to prevent SQL injection vulnerabilities. - Reason this comment was not posted:
Comment did not seem useful.
4. agents-api/agents_api/queries/docs/search_docs_by_text.py:19
- Draft comment:
Theowner_typesandowner_idsare passed as JSONB arrays, which might not be correctly handled by the SQL function. Ensure that these arrays are properly converted to UUID arrays to prevent unexpected behavior. - Reason this comment was not posted:
Comment did not seem useful.
5. agents-api/tests/test_docs_queries.py:11
- Draft comment:
Consider adding tests forsearch_docs_by_embeddingandsearch_docs_hybridto ensure comprehensive coverage of the search functionalities. - Reason this comment was not posted:
Confidence changes required:50%
Thetest_docs_queries.pyfile has a test forsearch_docs_by_textbut it lacks tests forsearch_docs_by_embeddingandsearch_docs_hybrid. Adding these tests would ensure comprehensive coverage of the search functionalities.
Workflow ID: wflow_qeLRxNLHNTGhklta
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Type
Enhancement
Description
Added comprehensive document management system with the following features:
Core document operations:
Advanced search capabilities:
List and filtering:
Performance optimizations:
Changes walkthrough 📝
9 files
__init__.py
Initialize document management module with core operationsagents-api/agents_api/queries/docs/init.py
create_doc.py
Document creation with ownership managementagents-api/agents_api/queries/docs/create_doc.py
constraints
delete_doc.py
Document deletion with ownership validationagents-api/agents_api/queries/docs/delete_doc.py
get_doc.py
Single document retrieval functionalityagents-api/agents_api/queries/docs/get_doc.py
list_docs.py
Paginated document listing with filtersagents-api/agents_api/queries/docs/list_docs.py
mmr.py
Maximal Marginal Relevance implementation for searchagents-api/agents_api/queries/docs/mmr.py
search_docs_by_embedding.py
Vector-based document search implementationagents-api/agents_api/queries/docs/search_docs_by_embedding.py
search_docs_by_text.py
Full-text document search implementationagents-api/agents_api/queries/docs/search_docs_by_text.py
search_docs_hybrid.py
Hybrid document search with score fusionagents-api/agents_api/queries/docs/search_docs_hybrid.py
Important
Add comprehensive document management system with CRUD, advanced search, and performance optimizations.
create_doc.py,delete_doc.py,get_doc.py, andlist_docs.py.search_docs_by_text.py.search_docs_by_embedding.py.search_docs_hybrid.py.mmr.py.Docmodel inDocs.pywith new fields likemodality,language,index,embedding_model, andembedding_dimensions.test_docs_queries.pyfor CRUD operations and listing.This description was created by
for 249513d. It will automatically update as commits are pushed.